Skip to content

[3.0] Recognise a gallery avatar as a file name, not a url - #9442

Merged
Sesquipedalian merged 1 commit into
SimpleMachines:release-3.0from
albertlast:3.0/avatar-prepackaged-persist
Aug 30, 2026
Merged

[3.0] Recognise a gallery avatar as a file name, not a url#9442
Sesquipedalian merged 1 commit into
SimpleMachines:release-3.0from
albertlast:3.0/avatar-prepackaged-persist

Conversation

@albertlast

Copy link
Copy Markdown
Collaborator

Description

An avatar picked out of the gallery is stored as a path relative to the avatars directory —
Oxygen/cards.png — not as a url. Avatar has no case for that, so it falls through to its
last resort, which pulls the path out of the url and looks for the file under the avatar
directories relative to the board directory.

That only lines up when the forum is installed under a path prefix. Two things go wrong
otherwise:

Every page is a fatal error. At the root of a domain there is no path to strip, and the
code reads it anyway:

preg_quote(Url::create(Config::$boardurl)->path, '~')

Url::$path is typed with no default and was never assigned, so:

Typed property SMF\Url::$path must not be accessed before initialization
  —  Sources/Avatar.php:539

One member with a gallery avatar takes down the board index, every topic they posted in,
the memberlist and their profile.

And the choice never sticks. Profile::setAvatarServerStored() puts the right value in
place — verified, new_data['avatar'] = 'Oxygen/cards.png' — but saving a member runs it
back through Avatar, and since that cannot resolve it, User::updateMemberData() writes
the column from whatever the object settled for instead. Set by hand, the avatar rendered
as default.png, and Avatar::$choice reported none, so the picker came back with
No avatar selected.

A string with no scheme is a file name. Saying so before the search starts lets it match on
attempt 2, where the prepackaged avatar directory is already handled, and the last-resort
url guessing is never reached.

Checked

http://localhost:8080 — a forum at the root of its domain.

Before, with smf_members.avatar = Oxygen/bug.png:

page
board index 500
their profile 500

After, saving each choice in turn through Profile → Forum Profile and re-reading the
profile:

chosen column rendered
gallery, nested (Oxygen/cookie.png) Oxygen/cookie.png …/avatars/Oxygen/cookie.png
gallery, top level (default.png) '' …/avatars/default.png
no avatar '' …/avatars/default.png
gravatar gravatar:// secure.gravatar.com/avatar/…
back to gallery (Oxygen/bug.png) Oxygen/bug.png …/avatars/Oxygen/bug.png

smf_log_errors stayed empty throughout. The top-level default.png storing as '' is
existing behaviour — User::updateMemberData() treats the default image as "no avatar".

This also settles the crash that #9440 guards against, by never reaching that branch for a
gallery avatar. #9440 is still worth having: it is the defensive fix for anything else that
gets that far with a pathless forum url.

Issues References (Fixes|Related|Closes)

Related #9440, #9441

An avatar picked out of the gallery is stored as a path relative to the
avatars directory: "Oxygen/cards.png", not a url. Avatar had no case for
that, so it fell through to the last resort, which pulls the path out of
the url and looks for the file under the avatar directories relative to
the board directory. That only lines up when the forum is installed
under a path prefix; at the root of a domain it reads
Url::create(Config::$boardurl)->path, which is a typed property that was
never assigned, and throws.

So a forum at the root of its domain was a fatal error on every page
showing a member with a gallery avatar - the board index included, by way
of the last-post line - and everywhere else the avatar came out as
default.png.

It also meant the choice never stuck. Profile::setAvatarServerStored()
puts the right value in place, but saving a member runs it back through
Avatar, and since that could not resolve it, the column was written with
whatever it had settled for instead.

A string with no scheme is a file name. Saying so before the search
starts lets it match on the second attempt, where the prepackaged avatar
directory is already handled.

Signed-off-by: Mathias Alberts <mathiaspapealbert@hotmail.com>
Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
@jdarwood007 jdarwood007 added this to the 3.0 Alpha 6 milestone Aug 9, 2026
Comment thread Sources/Avatar.php
Comment on lines +460 to +463
// scheme to it is a file name and not somewhere to fetch from. Saying
// so here means the search below finds it on its second attempt instead
// of guessing from the URL path, which only works out for a forum that
// is not at the root of its domain.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// scheme to it is a file name and not somewhere to fetch from. Saying
// so here means the search below finds it on its second attempt instead
// of guessing from the URL path, which only works out for a forum that
// is not at the root of its domain.
// scheme to it is a file name and not somewhere to fetch from.

@Sesquipedalian
Sesquipedalian merged commit b4f1820 into SimpleMachines:release-3.0 Aug 30, 2026
4 checks passed
@jdarwood007 jdarwood007 modified the milestones: 3.0 Alpha 6, 3.0 Alpha 5 Aug 30, 2026
live627 pushed a commit that referenced this pull request Aug 31, 2026
A second sweep of the bug fixes now on release-3.0, in the same spirit as
#9511: ask of each one whether the suite can reach it, and write a test where
it can. Everything merged since that sweep was looked at, along with the
backlog that landed in one batch on the 29th and 30th. Most of it is templates,
JavaScript, or PHP that wants Db::$db or User::$me. Four fixes do not.

#9484 made SMF\Unicode\SpoofDetector::checkReservedName() split the admin's
list on the two characters backslash and n as well as on a real newline. The
installer writes the default list with the separators spelled out that way, so
splitting on newlines alone gave one long name nobody would type and every
reserved name was free to register.

#9409 made SMF\Localization\MessageFormatter::formatMessage() flatten a
\Stringable argument to its string value. The class skips any argument that is
not already a string and hands the intl formatter only the scalar ones, so an
object argument reached neither and the member was shown the placeholder.

#9453 made SMF\PageIndex remember, across __toString(), that the start value it
was handed was out of bounds. fixStart() records that as a side effect of
clamping, and __toString() called it again on a value already clamped, so the
verdict was always thrown away: page 1 came out as plain text rather than a
link, with a "next page" link beside it.

#9440 and #9442 both concern a gallery avatar, which is stored as a path under
the avatars directory rather than as a URL. Read as a URL, it was worked back
to a file from the URL's path, which lands outside the avatar directories; and
on a forum at the root of its domain that path is null, so stripping the board
URL off it threw a TypeError on every page the member appeared on.

Each set was run against the code as it was before its fix, by checking out the
single source file at the commit before the merge:

- SpoofDetector.php before #9484: one failure, the installer's list.
- MessageFormatter.php before #9409: three failures, all the \Stringable cases.
  The plain string, the number and the no-placeholder message pass either side.
- PageIndex.php before #9453: two failures. The four tests covering an ordinary
  start pass either side, which is what makes them the control.
- Avatar.php before #9440: five of six fail, the root-of-domain cases with the
  TypeError and the subdirectory ones by falling through to default.png. With
  #9440 but not #9442, four still fail: every gallery avatar becomes the
  default image.

202 tests, 295 assertions, still under a second.

Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants